-
Notifications
You must be signed in to change notification settings - Fork 172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Computing LSTM forward layer without allocating #3351
Conversation
This comment was marked as spam.
This comment was marked as spam.
I can report perf improvement on th lstm from icu-perf
|
Yeah I got the 1% as well but it's nothing to write home about 😀 |
I'm all for celebrating little wins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this change
self.dic | ||
.get_copied(UnvalidatedStr::from_bytes(&buf[..i])) | ||
.get_copied_by(|key| { | ||
key.as_bytes().iter().copied().cmp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (optional): UTF-8 byte order is equivalent to UTF-32 order; it may be cleaner/smaller/faster code if you compared an iterator of char
rather than an iterator of u8
.
Separately, I'm starting to get a bit worried that the grapheme cluster code may bloat the code size of the LSTM segmenter. We should probably delete it at some point. Maybe as part of the next round of ML model upgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keys are UnvalidatedStr
s aka [u8]
, we cannot use them as anything that's strongly typed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is https://docs.rs/utf8_iter/latest/utf8_iter/ for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't like deps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segmenter already depends on utf8_iter
.
icu4x/components/segmenter/Cargo.toml
Line 35 in b6c4018
utf8_iter = "1.0.3" |
Based on #3349
There's no visible performance difference on our benchmarks, however this avoids an allocation of length
codepoints x hidden units
, and we only bench on small strings. For longer strings it's probably good to avoid this allocation.#3305